Add proper support for gitignore in AppWatcher #4917

isaacroldan · 2024-11-25T15:22:44Z

WHY are these changes introduced?

Fixes issues with file watching and extension creation detection during app dev.

WHAT is this pull request doing?

Improves file watching by using the ignore package to properly handle .gitignore files in extensions. If an extension defines it, anything in it will be ignored by the app watcher.
Fixes extension creation mid-dev by updating the file watcher when the app is reloaded.

How to test your changes?

Create a new app with some extensions
Add a .gitignore file to an extension directory
Start dev and verify:
- File changes matching .gitignore patterns are properly ignored
- New extensions are properly detected mid-dev

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes

isaacroldan · 2024-11-25T15:23:07Z

Add proper support for gitignore in AppWatcher #4917 👈 (View in Graphite)
Refactor FileWatcher into a Class #4916
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2024-11-25T16:27:36Z

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

github-actions · 2024-11-25T17:01:33Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	75.44% (+0.09% 🔼)	8740/11586
🟡	Branches	70.77% (+0.11% 🔼)	4266/6028
🟡	Functions	75.1% (+0.07% 🔼)	2289/3048
🟡	Lines	75.99% (+0.09% 🔼)	8264/10875

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / app-event-watcher.ts	93.83% (-1.11% 🔻)	86.49% (-2.08% 🔻)	90.48%	98.61% (+0.02% 🔼)

Test suite run success

1968 tests passing in 892 suites.

Report generated by 🧪jest coverage report action from 0442d42

isaacroldan · 2024-11-27T17:03:41Z

/snapit

github-actions · 2024-11-27T17:05:47Z

🫰✨ Thanks @isaacroldan! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/[email protected]

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

gracejychang · 2024-11-28T00:58:21Z

packages/app/src/cli/services/dev/app-events/app-event-watcher.ts

@@ -150,6 +151,7 @@ export class AppEventWatcher extends EventEmitter {
          if (!appEvent || appEvent.extensionEvents.length === 0) return

          this.app = appEvent.app
+          if (appEvent.appWasReloaded) this.fileWatcher?.updateApp(this.app)


just curious, how come we don't have to await for the fileWatcher to update the app here?

updateApp is not an async method, so we don't need to await :)

packages/app/src/cli/services/dev/app-events/file-watcher.test.ts

gonzaloriestra

I couldn't test it, but LGTM. Have you tested it in Windows? It's the typical thing that can be problematic there.

gonzaloriestra · 2024-11-29T10:29:30Z

packages/app/src/cli/services/dev/app-events/file-watcher.ts

@@ -93,15 +81,20 @@ export class FileWatcher {
  async start(): Promise<void> {
    const {default: chokidar} = await import('chokidar')

-    this.watcher = chokidar.watch(this.watchPaths, {
+    const extensionDirectories = [...(this.app.configuration.extension_directories ?? ['extensions'])]


What about including extension_directories in the app model and adding the default value there?

Also, I see we are not using the value from the configuration in some places, like here. That approach would simplify the correct usage everywhere.

gonzaloriestra · 2024-11-29T10:35:29Z

packages/app/src/cli/services/dev/app-events/file-watcher.ts

        '**/node_modules/**',
        '**/.git/**',
        '**/*.test.*',
        '**/dist/**',
        '**/*.swp',
        '**/generated/**',
-        ...this.customGitIgnoredPatterns,
+        '**/.gitignore',


I'd extract this list to a constant

isaacroldan · 2024-11-29T12:35:03Z

I did not, will do it before merging 🙏

isaacroldan mentioned this pull request Nov 25, 2024

Refactor FileWatcher into a Class #4916

Merged

3 tasks

isaacroldan force-pushed the 11-25-refactor_filewatcher_into_a_class branch from d975467 to 77a4f78 Compare November 25, 2024 15:27

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch from 68457a7 to 1aa04a1 Compare November 25, 2024 15:27

isaacroldan force-pushed the 11-25-refactor_filewatcher_into_a_class branch from 77a4f78 to 1b4002b Compare November 25, 2024 16:10

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch from 1aa04a1 to f675d44 Compare November 25, 2024 16:10

isaacroldan marked this pull request as ready for review November 25, 2024 16:27

isaacroldan requested a review from a team as a code owner November 25, 2024 16:27

isaacroldan force-pushed the 11-25-refactor_filewatcher_into_a_class branch from 1b4002b to 0899271 Compare November 26, 2024 09:53

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch from a045e02 to 9e17f4f Compare November 26, 2024 09:53

isaacroldan force-pushed the 11-25-refactor_filewatcher_into_a_class branch from 0899271 to 4536258 Compare November 27, 2024 12:18

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch from 9e17f4f to 6f07989 Compare November 27, 2024 12:18

isaacroldan force-pushed the 11-25-refactor_filewatcher_into_a_class branch from 4536258 to ff1bc64 Compare November 27, 2024 14:36

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch from 6f07989 to b78e49c Compare November 27, 2024 14:36

gracejychang reviewed Nov 28, 2024

View reviewed changes

packages/app/src/cli/services/dev/app-events/file-watcher.test.ts Outdated Show resolved Hide resolved

isaacroldan force-pushed the 11-25-refactor_filewatcher_into_a_class branch from ff1bc64 to 599e99b Compare November 28, 2024 10:02

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch 3 times, most recently from e12fef5 to da90c96 Compare November 28, 2024 10:41

isaacroldan changed the base branch from 11-25-refactor_filewatcher_into_a_class to graphite-base/4917 November 28, 2024 12:13

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch from da90c96 to d6f0844 Compare November 28, 2024 12:13

isaacroldan changed the base branch from graphite-base/4917 to main November 28, 2024 12:13

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch 2 times, most recently from d72edd6 to b2a69e8 Compare November 28, 2024 12:14

Add proper support for gitignore in AppWatcher

80ab0cd

isaacroldan added 2 commits November 29, 2024 10:28

Update tests

642bebd

Update tests

0442d42

isaacroldan force-pushed the 11-25-add_proper_support_for_gitignore_in_appwatcher branch from b2a69e8 to 0442d42 Compare November 29, 2024 09:29

gonzaloriestra approved these changes Nov 29, 2024

View reviewed changes

isaacroldan merged commit 55da34b into main Nov 29, 2024
28 checks passed

isaacroldan deleted the 11-25-add_proper_support_for_gitignore_in_appwatcher branch November 29, 2024 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proper support for gitignore in AppWatcher #4917

Add proper support for gitignore in AppWatcher #4917

isaacroldan commented Nov 25, 2024 •

edited

Loading

isaacroldan commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 •

edited

Loading

isaacroldan commented Nov 27, 2024

github-actions bot commented Nov 27, 2024

gracejychang Nov 28, 2024

isaacroldan Nov 28, 2024

gonzaloriestra left a comment

gonzaloriestra Nov 29, 2024

gonzaloriestra Nov 29, 2024

isaacroldan commented Nov 29, 2024

Add proper support for gitignore in AppWatcher #4917

Add proper support for gitignore in AppWatcher #4917

Conversation

isaacroldan commented Nov 25, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Checklist

isaacroldan commented Nov 25, 2024 • edited Loading

github-actions bot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 • edited Loading

Coverage report

Test suite run success

isaacroldan commented Nov 27, 2024

github-actions bot commented Nov 27, 2024

gracejychang Nov 28, 2024

Choose a reason for hiding this comment

isaacroldan Nov 28, 2024

Choose a reason for hiding this comment

gonzaloriestra left a comment

Choose a reason for hiding this comment

gonzaloriestra Nov 29, 2024

Choose a reason for hiding this comment

gonzaloriestra Nov 29, 2024

Choose a reason for hiding this comment

isaacroldan commented Nov 29, 2024

isaacroldan commented Nov 25, 2024 •

edited

Loading

isaacroldan commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading